grpc-protobuf: add server codegen utils - #2818
Conversation
cfdffb8 to
4640ba4
Compare
4640ba4 to
6ca5f3a
Compare
| /// Creates a new [`ServerStatusError`] from a [`StatusError`]. | ||
| pub fn from_status(status: StatusError) -> Self { |
There was a problem hiding this comment.
We should probably document this and into to explain why we didn't impl From instead.
There was a problem hiding this comment.
Added to the rustdocs of from_status and into_status explaining this.
| let mut res_view = ProtoRecvMessage::from_mut(req); | ||
| self.rx.dyn_next(&mut res_view).await |
There was a problem hiding this comment.
Nit: req_view since these are requests instead of responses? (I assume this was copied from the client recv_into which is why it's res?)
There was a problem hiding this comment.
Yes, it's a copy/paste error. Fixed.
| /// | ||
| /// Note: success does *not* indicate successful receipt of the response by | ||
| /// the client; it only indicates that the stream has not yet terminated. | ||
| pub async fn send(&mut self, resp: &impl AsView<Proxied = M>) -> Result<(), ()> { |
There was a problem hiding this comment.
Do we need to do something to silence clippy here?
There was a problem hiding this comment.
The clippy docs suggest replacing the unit struct () with a custom Err type that conveys what went wrong, for example:
#[derive(Debug)]
pub struct EndOfStream;
impl fmt::Display for EndOfStream {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "End of Stream")
}
}I was thinking of adding two types:
EndOfStream: Represents either a graceful termination or failure. This will be used in thesendmethod.UnexpectedEndOfStream: Represents stream failures. This will be used in therecvmethod that returnsOption<Result<M, ()>>presently, whereNonealready represents graceful termination.
We would need to make similar changes in the client APIs for consistency. What do you think?
|
|
||
| /// An adapter that wraps a [`BidiStreamingMethod`] to handle incoming | ||
| /// bidirectional-streaming RPCs. | ||
| pub struct BidiStreamingAdapter<M: BidiStreamingMethod> { |
There was a problem hiding this comment.
I think we're supposed to not specify the types here unless they're used here.
There was a problem hiding this comment.
Yes, removed the bounds from all the adapters and their impl blocks.
| { | ||
| return trailers_from_status(Err(ServerStatusError::new( | ||
| StatusCodeError::Internal, | ||
| "client did not send a request message", |
There was a problem hiding this comment.
This is the RPC error we send back to the client?
Maybe "unary call missing request message" or something?
Also should this be using an interceptor to deal with this (and also doing a second recv and expecting to get a None)?
| ))); | ||
| } | ||
|
|
||
| let mut resp = <M::Response as Default>::default(); |
There was a problem hiding this comment.
WDYT? Let's move this up to where req is defined and // TODO: allocate these together on an arena?
3 participants
This change introduces the types required for server codegen (see this branch for the full set of changes).
Additionally, this PR mvoes the
ServerStatusfrom thegrpccrate to thegrpc-protobufcrate.ServerStatuswraps the standard status type to prevent users from accidentally forwarding client errors from a server handler using the?operator.Finally, this change re-exports the
async_traitmacro from thegrpccrate (similar totonic). Becauseasync traitimplementations require#[async_trait], re-exporting it avoids requiring applications to addasync-traitas a direct dependency in theirCargo.tomlwhile using the generated code.